OpenConceptLab/ocl_online#425 | The announcement banner shows the deployment's ANNOUNCEMENT setting - #121

{
  "reviewer": "codex-cli 0.166.0",
  "perspective": "defects",
  "mode": "full",
  "commit": "41813d337b5073fa786ba68b556dce3e9870fa61",
  "verdict": "findings",
  "prior": [],
  "findings": [
    {
      "id": "F1.1",
      "severity": "P1",
      "tag": null,
      "file": "src/components/app/AnnouncementBanner.jsx",
      "line": 22,
      "claim": "The banner does not automatically disappear at its until time.",
      "scenario": "A user opens the app before until and keeps the page open past it. Expiry is checked only during rendering, with no timer triggering another render. A mounted-component reproduction confirmed that the expired banner and its layout offset remain until an unrelated render occurs.",
      "action": "Schedule a state update when until is reached, clean up the timer on unmount, and add a component test that advances past expiry without manually rerendering.",
      "lines_delta": null,
      "confidence": "high"
    }
  ]
}

{
  "reviewer": "codex-cli 0.166.0",
  "perspective": "defects",
  "mode": "verify",
  "commit": "a7e6d4995e76ec3479a14fd9a5b82dc6f7bd6ef1",
  "verdict": "clean",
  "prior": [
    {
      "id": "F1.1",
      "status": "resolved"
    }
  ],
  "findings": []
}

{
  "reviewer": "claude subagent",
  "perspective": "simplify",
  "mode": "full",
  "commit": "c55b3f9cbf1323306a64a7455f3e3499d5d83262",
  "verdict": "findings",
  "prior": [
    {
      "id": "F1.1",
      "status": "resolved"
    },
    {
      "id": "F1.2",
      "status": "resolved"
    }
  ],
  "findings": [
    {
      "id": "S1",
      "severity": null,
      "tag": "delete",
      "file": "oclweb3/src/common/announcement.js",
      "line": 37,
      "claim": "All three copies: the opening guard `if(typeof raw !== 'string' || !raw.trim()) return null` is redundant. The JSON.parse try/catch right below it already returns null for undefined, '', whitespace and any non-JSON value (JSON.parse coerces them and throws). isObject then rejects null, numbers and other non-object JSON. The validation stays; only the duplicate check goes.",
      "scenario": null,
      "action": "Nothing replaces it. I checked this on a scratch copy: the 'no setting: no banner' test and the other 17 still pass.",
      "lines_delta": -2,
      "confidence": "high"
    },
    {
      "id": "S2",
      "severity": null,
      "tag": "shrink",
      "file": "oclweb3/src/common/announcement.js",
      "line": 52,
      "claim": "All three copies: the links loop runs isObject(link) twice and builds the array by hand. localize already returns null for a label that isn't an object, and httpsURL returns null for a missing url, so `link?.` covers links that are strings, null or arrays. The file already uses `?.` (language?.split, announcement?.until), so this is safe for oclweb2's ESLint 4 + babel-eslint.",
      "scenario": null,
      "action": "Replace the 8-line loop with:\n  const links = setting.links.map(link => ({label: localize(link?.label, language), url: httpsURL(link?.url)}))\n  if(links.some(link => !link.label || !link.url))\n    return null\nAll link tests pass on a scratch copy.",
      "lines_delta": -5,
      "confidence": "medium"
    },
    {
      "id": "S3",
      "severity": null,
      "tag": "yagni",
      "file": "oclweb3/src/common/announcement.js",
      "line": 36,
      "claim": "All three copies: the `now = Date.now()` defaults on getAnnouncement and msUntilExpiry, and the `?.` on `announcement` in msUntilExpiry, guard cases no caller produces. AnnouncementBanner is the only caller. It always passes now and calls msUntilExpiry only when `open`, which means announcement is not null. Removing the `?.` also means removing the one-line `assert.equal(msUntilExpiry(null), null)` (test line 97). The assertions that pin F1.1 stay untouched: the one-hour delay, null for no until, and the re-check loop test.",
      "scenario": null,
      "action": "Use `(raw, language, now)` and `(announcement, now) => announcement.until ? ... : null`, and delete the msUntilExpiry(null) assertion. 18/18 pass on a scratch copy.",
      "lines_delta": -1,
      "confidence": "low"
    },
    {
      "id": "S4",
      "severity": null,
      "tag": "dup-test",
      "file": "oclweb3/src/common/__tests__/announcement.test.js",
      "line": 168,
      "claim": "All three copies: 'a setting cannot end the string and run script in env-config.js' repeats the checks of the round-trip test above it. That test already asserts Object.keys(context) is exactly ['ANNOUNCEMENT'] and the value reads back unchanged, and any payload that ends the string fails both checks. Merging keeps the same injection coverage with all four payloads.",
      "scenario": null,
      "action": "Move its four payloads into the round-trip test's value list and delete the separate test. On a scratch copy I removed each of js_string's four escape cases (\\\\, \\\", \\n, \\r) one at a time, and the merged test still failed every time.",
      "lines_delta": -8,
      "confidence": "medium"
    },
    {
      "id": "S5",
      "severity": null,
      "tag": "dup-test",
      "file": "oclweb3/src/common/__tests__/announcement.test.js",
      "line": 118,
      "claim": "All three copies: in the language test, the 9-line deepEqual for 'es' re-pins id, until and both URLs, which 'a valid setting shows its text and links' already covers. The only new behaviour it checks is the translated text and labels, including the en fallback for the second link's label.",
      "scenario": null,
      "action": "Replace it with:\n  assert.equal(announce(SETTING, 'es').text, 'Saturday maintenance, in Spanish.')\n  assert.deepEqual(announce(SETTING, 'es').links.map(link => link.label), ['Details, in Spanish', 'Contact us'])",
      "lines_delta": -7,
      "confidence": "medium"
    },
    {
      "id": "S6",
      "severity": null,
      "tag": "shrink",
      "file": "oclweb3/src/components/app/AnnouncementBanner.jsx",
      "line": 11,
      "claim": "All three copies (each one names the other two apps): the 4-line header comment repeats what announcement.js's header comment and the README already say: unset, malformed or expired means nothing shows; text is plain; a dismissal lasts until the id changes.",
      "scenario": null,
      "action": "Two lines: `// This deployment's announcement (common/announcement.js; README, \"Announcement banner\"),\n// fixed above the app bar. Same component in the Mapper and TBv2.`",
      "lines_delta": -2,
      "confidence": "low"
    }
  ]
}

{
  "reviewer": "codex-cli 0.166.0",
  "perspective": "defects",
  "mode": "final",
  "commit": "e764e292bc9faea30d0116d18859fb4440f18409",
  "verdict": "clean",
  "prior": [],
  "findings": []
}

{
  "reviewer": "codex-cli 0.166.0",
  "perspective": "defects",
  "mode": "final",
  "commit": "663e989ac2df24d0f181a3a17891b3a102c39cde",
  "verdict": "clean",
  "prior": [
    {
      "id": "A1",
      "status": "resolved"
    },
    {
      "id": "A2",
      "status": "resolved"
    }
  ],
  "findings": []
}

{
  "reviewer": "codex-cli 0.166.0",
  "perspective": "defects",
  "mode": "final",
  "commit": "d42da507dbc96b377dc444110d92652e8ecacb35",
  "verdict": "clean",
  "prior": [],
  "findings": []
}